Looks like there's some tests that need to be fixed after pulling in latest main

Hey @JustinGrote I've updated the tests to use Async, they should at least run / compile now

Latest MacOS/Ubuntu test failures look like a path combine issue.
System.IO.DirectoryNotFoundException : Could not find a part of the path '/Users/runner/work/PowerShellEditorServices/PowerShellEditorServices/test/PowerShellEditorServices.Test.Shared/Refactoring\Utilities/TestDetection.ps1'.

Note the backslash after Refactoring, may want to make sure you are using Path.Combine or Join-Path

EDIT: Found the backslash hardcoded, refactored to use the three-argument path.combine, this should still work with net462

OK, all tests pass in CI now :). I'll do my best to get a first-pass review done sometime this week, and provide devcontainer/codespaces instructions to allow people to test for edge cases we need to document.

Looking pretty good so far in terms of design, some cleanup will be needed.

Several foreach rename issues I've already come across, I'll try to define tests for these when I get a chance.

#Doesnt rename testvar
$a = 1..5
$b = 6..10
function test {
    process {
        foreach ($testvar in $a) {
            $testvar
        }
        
        foreach ($testvar in $b) {
            $testvar
        }
    }
}

#Renames both foreach local variables ($aPath), should only rename the one in scope (agreed this is bad practice though)
function Import-FileNoWildcard {
    [CmdletBinding(SupportsShouldProcess=$true)]
    param(
        # Specifies a path to one or more locations.
        [Parameter(Mandatory=$true,
                   Position=0,
                   ParameterSetName="Path",
                   ValueFromPipeline=$true,
                   ValueFromPipelineByPropertyName=$true,
                   HelpMessage="Path to one or more locations.")]
        [Alias("PSPath", "Path")]
        [ValidateNotNullOrEmpty()]
        [string[]]
        $Path2
    )

    begin {
    }

    process {
        # Modify [CmdletBinding()] to [CmdletBinding(SupportsShouldProcess=$true)]
        $paths = @()
        foreach ($aPath in $Path2) {
            if (!(Test-Path -LiteralPath $aPath)) {
                $ex = New-Object System.Management.Automation.ItemNotFoundException "Cannot find path '$aPath' because it does not exist."
                $category = [System.Management.Automation.ErrorCategory]::ObjectNotFound
                $errRecord = New-Object System.Management.Automation.ErrorRecord $ex,'PathNotFound',$category,$aPath
                $psCmdlet.WriteError($errRecord)
                continue
            }

            # Resolve any relative paths
            $paths += $psCmdlet.SessionState.Path.GetUnresolvedProviderPathFromPSPath($aPath)
        }

        foreach ($aPath in $paths) {
            if ($pscmdlet.ShouldProcess($aPath, 'Operation')) {
                # Process each path
                $aPath
            }
        }
    }

    end {
    }
}

We will also need to make sure we account for the scoping issues as mentioned by Rob that are very difficult to handle, even if it means we warn about this as best effort, or straight up refuse to do it.
PowerShell/vscode-powershell#261 (comment)

I made some test cases for renaming from within a for / each loop and limiting the rename to the scope if the var is defined within the creation of the statement. My only concern is a case like below.

If you run the code powershell treats the $i = 10 as the highest scope assignment which is then redefined in the loop but kept in function scope for when its printed. As it stands if you renamed $i = 10 it would rename the loop using the same variable. however if you renamed from within the loop it would not touch the $i=10

$a = 1..5
$b = 6..10
function test {
    process {
        $i = 10
        
        for ($Renamed = 0; $Renamed -lt $b.Count; $Renamed++) {
            $null = $Renamed
        }
        
        for ($i = 0; $i -lt $a.Count; $i++) {
            write-output $i
        }
        
        write-output "I will be 5 : $i not 10"
    }
}
test

@Razmo99 uh I don't know what I did but a bunch of non-matching main commits showed up, it's not letting me force push a reset, can you please revert to ff9c25b?

EDIT: Nevermind fixed it!

Just installed. It renamed the function in the file that was open. VSCode saw a reference in a nother file but the reference wasn't renamed. I also noticed it didn't add the Alias attribute. Let me know if there's any other info I could provide to help.

Also checked if I could rename from where it was used. But that gave me an error about the function not being defined.

I don't see anything in the PowerShell or the PowerShell: LSP Trace output panels with logging turned up.

VSCode saw a reference in another file but the reference wasn't renamed.

As was explained in the README that you clicked "I Accept" on, rename is limited to a single file only :). Whereas the references may or may not be able to find cross-file references, there's no guarantee those are the same reference, and that's fine when read-only but potentially disastrous when making changes.

Also the Alias is now an opt-in setting, but I think it needs a little more work, you'll need to check settings for it and enable it.

As was explained in the README that you clicked "I Accept" on,

Should have RTFM. My bad! This seems like the kind of thing that would be nice to have opt-in. It requires an extra step so users have to explicitly decide and at that point we can warn them of the possible issues (e.g. ensuring all the code base is loaded into AST, how to check if all the references are correct, how to exclude files from being referenced, etc.). With the rename functionality you also have the option to hit Ctrl + Enter to see the Refactor Preview pane. That shows you a list of changes and you can choose which to apply.

Also the Alias is now an opt-in setting

Why opt in? If we start with the assumption that the user isn't familiar with the Alias attribute, at worst they have an extra/unused line of code. This may prompt them to investigate what it is and then make an informed decision. Compared to a user not knowing and not realizing they can offer a simple backwards compatible feature.

Should have RTFM. My bad! This seems like the kind of thing that would be nice to have opt-in.

I know, but the amount of unit tests to cover a proper implementation, not to mention what the model would have to look like, is extremely ugly, only to have it only ever be a half-baked, "works sometimes" implementation. There's just too many ways that dot sourcing etc. break this model. It might be somewhat feasible for functions in a best effort manner given the same limitations, but for variables it's far too risky as people reuse variable names within functions constantly.

The intent of this feature is for simple refactors within a single file, e.g. renaming a variable within a function. It's not meant for multi-file project-wide refactors. You really need a type-safe language for something like that to be effective which PowerShell isn't.

Why opt in? If we start with the assumption that the user isn't familiar with the Alias attribute, at worst they have an extra/unused line of code. This may prompt them to investigate what it is and then make an informed decision. Compared to a user not knowing and not realizing they can offer a simple backwards compatible feature.

During development a lot of people felt the ergonomics of the Alias attribute coming in was annoying, as they weren't looking for backwards compatibility, just to rename a variable/parameter of a function-in-development, so more often than not they were going back and deleting the alias than it being actually useful.

As an example for variables, here's what Find All References returns:

Should we rename all of these $result? They aren't all the same variable.

Same with Parameters. Finding all references across files with splatting, etc. is extremely difficult. What if you defined your splat in one file and reference it in another? Do we need to cross correlate your dotsources? How do we know those are accurate?

Function definitions and usage have their issues, but tend to be fairly "unique" to a module/project, but if you have two modules in the same repo with their own internal function definitions, how do we find the module "boundary"?

And these are just the problems I can think of off the top of my head. As mentioned in the above, the major risk here is turning this into a "feature/bug farm". If I enable function renaming across files, immediately we are going to get requests for variable rename across files, and then people will be disappointed when it won't work, so we have to establish certain reasonable boundaries. Maybe we will enable function renaming across files as an opt-in feature in the future, but that's another set of scenarios and unit tests etc. and there's only so much I'm willing to do with my volunteer time on this vs. other pressing issues :)